-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Site Editor: add more menu and fullscreen toggle #21006
Conversation
Size Change: -412 B (0%) Total Size: 857 kB
ℹ️ View Unchanged
|
min-width: 260px; | ||
|
||
// Let the menu scale to fit items. | ||
@include break-mobile() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably copied, but that particular breakpoint is not something we use often, I guess we prefer "small/medium"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know the reasons for these styles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh I don't. I just copied the CSS file from the existing component and made a note to circle back and check if all of this is needed. I tried testing on mobile widths but couldn't notice any visual difference.
I removed this in 5582081 but let me know if you'd prefer me to do something else.
@@ -1,3 +1,5 @@ | |||
@import "./more-menu/style.scss"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try to import things in the main style.scss of the package for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 5582081.
} | ||
|
||
export default compose( [ | ||
withSelect( ( select, { feature } ) => ( { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: but we're preferring useSelect/useDispatch lately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Refactored in 093b683.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good, left some minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great and works great on my end!
I have nothing to point out code-wise that Riad hasn't.
I am curious. Why isn't the standard "more" menu available here? I did test this and it works as expected. |
I guess no other reason than the fact that no one has developed it yet, and since this is a separate editor instance it's not something that will be carried over automatically. I suppose that we'll introduce it in the near future, at least for the items that make sense in this context. |
That makes sense. I suppose the next question is about whether this should be a separate editor instance (not related to this PR) or whether the "more" menu should be a component that can be reused consistently. Something to chew on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great
Description
Adds more menu to the Site Editor's header which contains the Fullscreen mode toggle option.
Follow up of #20691
How has this been tested?
Navigate to site editor and verify that Fullscreen toggle works as expected. Reload the page and make sure that the preference is saved properly.
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: